Skip to content

Speed up TLV serialization length accounting#4642

Open
joostjager wants to merge 2 commits into
lightningdevkit:mainfrom
joostjager:ser-tlv-length-fastpath
Open

Speed up TLV serialization length accounting#4642
joostjager wants to merge 2 commits into
lightningdevkit:mainfrom
joostjager:ser-tlv-length-fastpath

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented May 28, 2026

Recent splice fuzzing coverage made it clear that some chanmon consistency inputs spend a surprising amount of time encoding snapshots. This PR tightens the serialization length path used by those snapshots.

Slow fuzz byte string used while checking this:

a1a0a3ffa2a1ffffa2a1ff00a2a1ffa2a1a0a3ffa2a1ff19ffffffffffffffac

For that byte string, median real time over 5 runs with target output suppressed:

Profile Pre changes Post changes
Debug 17.17s 3.61s
Release 0.60s 0.24s

@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented May 28, 2026 •
edited
Loading

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager joostjager requested a review from wpaulino May 28, 2026 14:28
@joostjager joostjager marked this pull request as ready for review May 28, 2026 14:28
@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

ldk-claude-review-bot commented May 28, 2026 •
edited
Loading

I've completed a thorough review of all changes in this PR. Here's my analysis:

Verification points checked:

  1. _tlv_fields_serialized_length! macro: Computes len + BigSize(len).serialized_length(), matching exactly what _encode_varint_length_prefixed_tlv! writes (BigSize prefix + TLV stream body).

  2. impl_writeable_tlv_based! refactoring: The old inlined length code and the new _tlv_fields_serialized_length! delegation are equivalent — both pass &self.field_name to _get_varint_length_prefixed_tlv_length! (the old code passed &self.$field directly, the new code passes self.$field and the macro adds &).

  3. impl_writeable_only_tlv_fields!: Write path uses write_tlv_fields! and length path uses _tlv_fields_serialized_length!. Both receive the same field expressions. The $self ident is not forwarded to inner macros, which is consistent — the write path (write_tlv_fields!) also doesn't forward it. Current usages (required, option, required_vec, (required: ReadableArgs, ...)) don't need it.

  4. Option<T>::serialized_length: None → 1 (matching 0u8.write()). Some(data) → BigSize(data_len + 1).serialized_length() + data_len (matching BigSize(data_len + 1).write() + data.write()).

  5. WithoutLength<S>::serialized_length: Iterates elements calling serialized_length(), matching the write path that iterates calling write().

  6. &T and Box<T> overrides: Both delegate to inner type, matching their write implementations.

  7. debug_assert_eq! safety net in encode() (ser.rs:289) validates at test time that any serialized_length() override matches actual write() output.

  8. legacy_deserialization_prevention_marker inlining: Previously a local variable (evaluated once), now an expression (evaluated twice in write path — once for length calc, once for encoding). The function is pure and deterministic, so this is safe.

No issues found.

@joostjager joostjager force-pushed the ser-tlv-length-fastpath branch from 7afe504 to 084d0ac Compare May 28, 2026 15:21
@joostjager joostjager self-assigned this May 28, 2026
wpaulino
wpaulino reviewed May 28, 2026
View reviewed changes
Comment thread lightning/src/util/ser_macros.rs Outdated
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented May 28, 2026

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@joostjager joostjager force-pushed the ser-tlv-length-fastpath branch from 084d0ac to b0e52bd Compare May 28, 2026 17:41
@joostjager joostjager requested a review from wpaulino May 28, 2026 18:01
tnull
tnull reviewed May 29, 2026
View reviewed changes
Comment thread lightning/src/util/ser_macros.rs
TheBlueMatt
TheBlueMatt reviewed May 29, 2026
View reviewed changes
Comment thread lightning/src/util/ser_macros.rs Outdated
Comment thread lightning/src/util/ser.rs Outdated
Comment thread fuzz/fuzz-fake-hashes/src/bin/zbase32_target.rs Outdated
let mut data = Vec::with_capacity(8192);
std::io::stdin().read_to_end(&mut data).unwrap();
zbase32_test(&data, test_logger::Stdout {});
if std::env::var_os("LDK_FUZZ_SUPPRESS_LOGS").is_some() {
Copy link
Copy Markdown
Collaborator

@TheBlueMatt TheBlueMatt May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bleh. Tell claude env variables for runtime decisions is gross and not the clean solution it thinks it is. If the point of stdin-fuzz is to be used to replicate tests, we should leave stdout, if its purpose is as a fuzz target, it should be DevNull.

Copy link
Copy Markdown
Contributor Author

@joostjager joostjager May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted this, not Claude 😅 I use stdin_fuzz also to run big sets of test cases using a runner that parallelizes and also continues on failure to examine the rest. That wouldn't work using the cargo test and test_cases dir approach.

Copy link
Copy Markdown
Contributor Author

@joostjager joostjager May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to other suggestions for a convenient way to feed strings without intermediate files, while being able to switch logging on or off.

Copy link
Copy Markdown
Contributor Author

@joostjager joostjager May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the time being, moved to #4646 to unblock this PR.

@joostjager joostjager force-pushed the ser-tlv-length-fastpath branch from b0e52bd to 78817b6 Compare May 29, 2026 10:36
@joostjager joostjager requested a review from TheBlueMatt May 29, 2026 10:40
@joostjager joostjager force-pushed the ser-tlv-length-fastpath branch from 78817b6 to 2060cff Compare May 29, 2026 10:45
@joostjager
Copy link
Copy Markdown
Contributor Author

joostjager commented May 29, 2026 •
edited
Loading

Review comments addressed: https://github.com/lightningdevkit/rust-lightning/compare/b0e52bd..7dd5bca

Dropped commit, so no clean diff is possible anymore.

@joostjager joostjager removed the request for review from TheBlueMatt May 29, 2026 10:48
@joostjager joostjager force-pushed the ser-tlv-length-fastpath branch 3 times, most recently from 7dd5bca to 70b69e4 Compare May 29, 2026 13:35
joostjager added 2 commits May 29, 2026 15:35
@joostjager
Avoid nested TLV length counting writes
f188957 
Add direct serialized length implementations for common serialization
wrappers. This avoids routing field payload length calculations through
in-memory writers for common nested serialization paths used by the
existing TLV length helpers.
@joostjager
Generate TLV write length impls
18a137a 
Add a write-only TLV helper macro that emits both write and
serialized_length from the same field list. Reuse the shared TLV length
helper from impl_writeable_tlv_based so the existing generated writer
path and the new custom-read path stay aligned.

Use the new helper for the hot channel funding and commitment
transaction TLV writers while leaving their custom read implementations
unchanged.
@joostjager joostjager force-pushed the ser-tlv-length-fastpath branch from 70b69e4 to 18a137a Compare May 29, 2026 13:36
@joostjager joostjager requested a review from TheBlueMatt May 29, 2026 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tnull tnull tnull left review comments

@wpaulino wpaulino Awaiting requested review from wpaulino

@TheBlueMatt TheBlueMatt Awaiting requested review from TheBlueMatt

At least 1 approving review is required to merge this pull request.

Assignees

@joostjager joostjager

Labels

None yet

Projects

Weekly Goals
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joostjager @ldk-reviews-bot @ldk-claude-review-bot @tnull @TheBlueMatt @wpaulino